fix(mail): retry instead of ack on lease-held reconcile; token-scope lease release (HT-48) - #49
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds mailbox-scoped reconciliation leases backed by ChangesGmail reconciliation lease
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReconcileConsumer
participant WatchStateStore
participant GmailAPI
ReconcileConsumer->>WatchStateStore: Claim mailbox lease
alt Claim fails
WatchStateStore-->>ReconcileConsumer: No token
ReconcileConsumer-->>ReconcileConsumer: Return retry with backoff
else Claim succeeds
WatchStateStore-->>ReconcileConsumer: Lease token
ReconcileConsumer->>GmailAPI: List history and fetch messages
ReconcileConsumer->>WatchStateStore: Advance cursor
ReconcileConsumer->>WatchStateStore: Release matching lease token
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mail/gmail-reconcile.ts`:
- Around line 432-505: The reconciliation cursor is read before lease
acquisition and advanced without fencing, allowing an expired holder to use
stale state or regress a successor’s cursor. In the reconciliation flow around
claimReconcileLease, re-read the cursor after obtaining leaseToken and use that
post-claim cursor for history.list; replace unconditional setCursor advancement
with an atomic lease-token-conditioned update that rejects stale holders. Add a
fixture covering a successor committing before the expired holder, preserving
monotonic cursor behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 030ad557-9356-4008-979e-0e6096f20bab
📒 Files selected for processing (9)
specs/mail/gmail-push.mdsrc/db/migrate.test.tssrc/db/migrate.tssrc/mail/gmail-connect.test.tssrc/mail/gmail-reconcile.test.tssrc/mail/gmail-reconcile.tssrc/mail/gmail-watch-maintenance.tssrc/store/gmail-watch-state.test.tssrc/store/gmail-watch-state.ts
Comment on lines
+432
to
+505
| // --- Step 3a: claim the reconciliation lease (HT-48; module doc's "The | ||
| // reconciliation lease" section). A run that cannot claim it retries | ||
| // shortly rather than acking — module doc's "Why a failed claim retries | ||
| // instead of acking" explains why acking here can silently drop a | ||
| // message that arrived after the holder's own history.list snapshot. --- | ||
| const leaseToken = await watchStateStore.claimReconcileLease(mailboxId, reconcileLeaseMs) | ||
| if (leaseToken === null) { | ||
| logReconcileEvent('info', { | ||
| mailboxId, | ||
| outcome: 'ack', | ||
| reason: 'cursor-expired', | ||
| cursor, | ||
| note: 'cursor expired (404); mailbox paused for manual rebaseline per gmail-push.md §5', | ||
| outcome: 'retry', | ||
| reason: 'reconcile-lease-held', | ||
| backoffSeconds: reconcileLeaseRetryBackoffSeconds, | ||
| note: "another in-flight reconcile (push or sweep) holds this mailbox lease; retrying shortly rather than acking, so anything past the holder's own history.list snapshot is not silently dropped — gmail-push.md §6, HT-48", | ||
| }) | ||
| return { kind: 'ack' } | ||
| return { kind: 'retry', backoffSeconds: reconcileLeaseRetryBackoffSeconds } | ||
| } | ||
|
|
||
| // --- Step 5: fetch + ingest each added message, in order. --- | ||
| const outcomes: IngestOutcome[] = [] | ||
| for (const messageId of listed.messageIds) { | ||
| const fetched = await client.getRawMessage(messageId) | ||
| if (fetched === null) { | ||
| // Deleted between list and get — nothing to ingest, nothing to | ||
| // retry; skip (module doc, step 5). | ||
| continue | ||
| try { | ||
| // --- Step 4: history.list from the stored cursor. --- | ||
| const client = createHistoryClient(getAccessToken) | ||
| const listed = await client.listAddedMessageIds(cursor) | ||
| if (listed.kind === 'expired') { | ||
| await mailboxStore.markPaused(mailboxId) | ||
| logReconcileEvent('warn', { | ||
| mailboxId, | ||
| outcome: 'ack', | ||
| reason: 'cursor-expired', | ||
| cursor, | ||
| note: 'cursor expired (404); mailbox paused for manual rebaseline per gmail-push.md §5', | ||
| }) | ||
| return { kind: 'ack' } | ||
| } | ||
|
|
||
| const content = await buildRawMessageContent(fetched.rawBytes, { | ||
| mailboxId, | ||
| messageId, | ||
| maxInlineRawBytes, | ||
| blobStore, | ||
| }) | ||
| // --- Step 5: fetch + ingest each added message, in order. --- | ||
| const outcomes: IngestOutcome[] = [] | ||
| for (const messageId of listed.messageIds) { | ||
| const fetched = await client.getRawMessage(messageId) | ||
| if (fetched === null) { | ||
| // Deleted between list and get — nothing to ingest, nothing to | ||
| // retry; skip (module doc, step 5). | ||
| continue | ||
| } | ||
|
|
||
| const raw: RawInboundMessage = { | ||
| content, | ||
| mailboxId, | ||
| providerMessageId: messageId, | ||
| receivedAt: fetched.receivedAt, | ||
| const content = await buildRawMessageContent(fetched.rawBytes, { | ||
| mailboxId, | ||
| messageId, | ||
| maxInlineRawBytes, | ||
| blobStore, | ||
| }) | ||
|
|
||
| const raw: RawInboundMessage = { | ||
| content, | ||
| mailboxId, | ||
| providerMessageId: messageId, | ||
| receivedAt: fetched.receivedAt, | ||
| } | ||
| outcomes.push(await ingest(raw)) | ||
| } | ||
| outcomes.push(await ingest(raw)) | ||
| } | ||
|
|
||
| // --- Step 6: advance the cursor iff every outcome is terminal & ledgered. --- | ||
| const blocking = outcomes.find((o) => o.kind === 'failed' || o.kind === 'in-progress') | ||
| if (blocking !== undefined) { | ||
| logReconcileEvent('warn', { | ||
| // --- Step 6: advance the cursor iff every outcome is terminal & ledgered. --- | ||
| const blocking = outcomes.find((o) => o.kind === 'failed' || o.kind === 'in-progress') | ||
| if (blocking !== undefined) { | ||
| logReconcileEvent('warn', { | ||
| mailboxId, | ||
| outcome: 'retry', | ||
| reason: 'non-terminal-ingest-outcome', | ||
| blockingOutcomeKind: blocking.kind, | ||
| blockingProviderMessageId: blocking.providerMessageId, | ||
| batchSize: listed.messageIds.length, | ||
| }) | ||
| return { kind: 'retry' } | ||
| } | ||
|
|
||
| await watchStateStore.setCursor(mailboxId, listed.newHistoryId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fence cursor reads and advancement with the lease token.
If holder A exceeds leaseMs, successor B can claim and persist newer cursor H2, after which A unconditionally writes its older H1 at Line 505. The cursor can therefore regress despite its monotonic contract. A claimant can also use a stale cursor because Line 420 reads it before the claim.
Re-read the cursor after claiming and make advancement an atomic token-conditioned update. Add a fixture where a successor commits before the expired holder.
As per coding guidelines, “Treat mail semantics as sacred: changes affecting them require fixture-proven equivalence or explicit written justification.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mail/gmail-reconcile.ts` around lines 432 - 505, The reconciliation
cursor is read before lease acquisition and advanced without fencing, allowing
an expired holder to use stale state or regress a successor’s cursor. In the
reconciliation flow around claimReconcileLease, re-read the cursor after
obtaining leaseToken and use that post-claim cursor for history.list; replace
unconditional setCursor advancement with an atomic lease-token-conditioned
update that rejects stale holders. Add a fixture covering a successor committing
before the expired holder, preserving monotonic cursor behavior.
Source: Coding guidelines
Serialize push-triggered reconcile (HT-41) and the daily sweep (HT-42) per mailbox to avoid redundant history.list/messages.get work, without touching correctness: a run that cannot claim the lease skips and acks (the holder will advance the cursor); different mailboxes still reconcile concurrently. - migration 016 adds gmail_watch_state.claimed_until, mirroring the outbound delivery lease (threads.claimed_until, migration 003). - GmailWatchStateStore gains claimReconcileLease/releaseReconcileLease, an atomic claim (UPDATE ... WHERE claimed_until IS NULL OR < now()) with no status re-check, since this lease guards no outcome, only Gmail API work. - gmail-reconcile.ts claims the lease after confirming a stored cursor and before history.list, and releases it in a finally around the fetch/ ingest/cursor-advance block so release happens on every exit, including a thrown error, before the handler's own top-level catch runs. This is a deliberate choice: because the lease is a pure efficiency guard, a crash must never lock a mailbox out of reconciliation until the lease's own expiry backstop; releasing immediately means the next trigger can proceed right away instead. - Tests cover concurrent reconcile of one mailbox (Gmail work happens once, second run skips), concurrent reconcile of different mailboxes (never blocked by each other), an expired lease being claimable, and a crashed holder's dangling lease expiring so reconciliation resumes. - Updates gmail-push.md §6 and gmail-watch-maintenance.ts's stale "deferred to HT-48" comments to record the lease as implemented in the reconcile consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lease release (HT-48)
Review findings on the HT-48 reconciliation lease:
- A failed lease claim now returns {kind:'retry', backoffSeconds} instead
of acking. Acking silently dropped any message whose history record
postdates the current holder's history.list snapshot (e.g. a push
notification consumed by a second run while the first still holds the
lease) — up to ~24h of added latency on a quiet mailbox. The backoff
(DEFAULT_RECONCILE_LEASE_RETRY_BACKOFF_SECONDS) is sized so the queue's
own exponential-backoff/maxAttempts window comfortably outlasts the
lease's max hold time before dead-lettering.
- claimReconcileLease now returns an opaque lease token (claimed_until
rendered as text, to avoid a JS Date's millisecond-precision truncation
of a microsecond-precision timestamptz) instead of a boolean;
releaseReconcileLease takes that token and only clears the lease if it
still matches the row's current claimed_until, otherwise it is a silent
no-op. This closes a stale-holder hole where a run that overran its
lease (e.g. a large post-downtime backlog) could release and clobber a
legitimate successor's live lease.
Updates specs/mail/gmail-push.md §6 and the migration 016 doc comment to
match. No new dependencies, no schema change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zaridan
force-pushed
the
feat/ht-48-gmail-reconcile-lease
branch
from
July 16, 2026 22:23
7f48440 to
0c349c7
Compare
Contributor
Author
|
@coderabbitai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
history.list/messages.getredundantly for the same mailbox at the same time, while different mailboxes still reconcile concurrently.gmail_watch_state.claimed_until, mirroring the outbound delivery lease (threads.claimed_until, migration 003).GmailWatchStateStoregainsclaimReconcileLease/releaseReconcileLease, an atomic claim (UPDATE ... WHERE claimed_until IS NULL OR < now()) with no status re-check, since this lease guards no correctness outcome, only redundant Gmail API work.gmail-reconcile.tsclaims the lease after confirming a stored cursor and beforehistory.list, and releases it in afinallyaround the fetch/ingest/cursor-advance block so release happens on every exit — including a thrown error — before the handler's own top-level catch runs.{ kind: 'retry', backoffSeconds }instead of acking. Acking on a failed claim silently dropped any message that arrived in Gmail's history after the current holder'shistory.listsnapshot — up to ~24h of added latency on an otherwise-quiet mailbox (next reconciled only on the next push or the daily sweep). The retry backoff (DEFAULT_RECONCILE_LEASE_RETRY_BACKOFF_SECONDS) is sized so the queue's own exponential backoff/maxAttemptsdead-letter window comfortably outlasts the lease's max hold time, guaranteeing at least one retry after the holder is certain to have released.claimReconcileLeasenow returns an opaque lease token (claimed_untilrendered as::text, to avoid a JSDate's millisecond-precision truncation of a microsecond-precisiontimestamptz) instead of a boolean.releaseReconcileLeasetakes that token and only clears the lease if it still matches the row's currentclaimed_until, otherwise it's a silent no-op. This closes a stale-holder hole where a run that overran its lease (e.g. a large post-downtime backlog) could release and clobber a legitimate successor's live lease.specs/mail/gmail-push.md§6 and the migration 016 doc comment to match the corrected behavior. No new dependencies, no schema change beyond the single new nullableclaimed_untilcolumn.Design decisions
history.listcall rather than a silent skip. No sign-off blocker identified, but calling it out since it touches mail-delivery-adjacent latency (charter §2 territory).DEFAULT_RECONCILE_LEASE_RETRY_BACKOFF_SECONDS = 25) is explicitly documented in the code as a judgment call, sized againstDEFAULT_RECONCILE_LEASE_MS(5 min) and the queue's defaultmaxAttempts/backoff growth so the total retry window (~6.25 min) safely outlasts the lease. IfreconcileLeaseMsis ever overridden well above its default at the composition root, this constant (or the queue'smaxAttempts/backoff options) should be reconsidered alongside it — flagging for the maintainer's awareness, not asking for a decision now, since the default values ship unchanged.::text/::timestamptzround-trip (not a JSDate) is a deliberate precision-safety choice, documented ingmail-watch-state.ts's doc comment, to avoid a lossy millisecond truncation reintroducing the exact lock-out the token exists to prevent.Review
Independent gate: typecheck 0, lint 0, tests 0 (exit codes). Adversarial review of record: 6 findings (4 actionable), fixes applied and re-gated.
Verification
Ran in
/Users/tjbaker/Projects/helpthread-worktrees/feat-ht-48-gmail-reconcile-lease. All exit codes verified directly (viaecho $?/marker file reads, not via background-task notifications):git status --porcelain-> empty output, exit 0 -> tree clean.npm run typecheck(tsc --noEmit -p tsconfig.json) -> exit 0, no errors.npm run lint(biome check .) -> exit 0. Output: "Checked 180 files in 112ms. No fixes applied."vitest run-> exit 0, 765/765 tests passing, 41/41 files passing (Test Files 41 passed (41)/Tests 765 passed (765)). The default-config run on this shared dev box (concurrent HT-46/HT-47 gate runs also in flight, load average >100 on 10 cores) produced spuriousTest timed out in 20000ms/Hook timed out in 10000msfailures — all in PGlite-per-test-file setup/beforeEachunder contention, never an assertion failure, and never in the two files this PR actually touches (gmail-reconcile.test.ts,gmail-watch-state.test.ts— both green on every run). Re-ran with--testTimeout=60000 --hookTimeout=60000to give the same assertions more wall-clock headroom under that contention; the final run came back fully clean with no timeout inflation left needed to explain. Confirmed clean tree again after the run (git status --porcelain-> empty, exit 0).origin/mainbefore pushing (git merge-base HEAD origin/main==origin/mainHEAD, 0 commits behind) — no rebase needed.🤖 Generated with Claude Code
Summary by CodeRabbit